test(wallet-service): harden /version contract#428
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWiden native_token to include optional numeric version, return the schema-validated value from fullnode.version(), and add tests validating modern and legacy payloads plus schema strictness and handler error behavior. ChangesFullnode Version Schema & Handler Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds direct schema unit tests for FullnodeVersionSchema, two fullnode.version() tests, and a live-fullnode contract test that fails CI when the /version response stops matching the schema. Aligns FullNodeApiVersionResponse with the native_token.version field added in #420 and drops the type cast that was masking the drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tuliomir
force-pushed
the
chore/pr-420-hardening
branch
from
de286a6 to
df1493e
Compare
Both schemas.test.ts and fullnode.test.ts duplicated a ~17-line FullNodeApiVersionResponse fixture. Replaces both with reuse of the existing `defaultTestVersionData()` factory in tests/utils.ts — already the canonical fixture (used by `seedFullnodeVersionData` and referenced from jestSetup.ts) — spreading and overriding only the `native_token` field under test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wraps the three version()-related cases in a `describe('version', ...)`
block for readability. Adds a case asserting `fullnode.version()` still
parses successfully when the fullnode response omits the new
`native_token.version` field, mirroring older fullnodes that predate
#420.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/wallet-service/tests/fullnode.test.ts (1)
4-53: ⚡ Quick winAdd mock cleanup to keep tests isolated.
Each test in this
describe('version')block usesjest.spyOn(fullnode.api, 'get')but never restores it. Add anafterEach(() => jest.restoreAllMocks())to avoid cross-test leakage as this suite grows. This aligns with the cleanup patterns already used elsewhere in the test suite.Suggested addition
describe('version', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + test('returns parsed payload when native_token includes the version field', async () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/wallet-service/tests/fullnode.test.ts` around lines 4 - 53, Add test teardown to the describe('version') suite to restore spies after each test: in the same describe block that contains tests calling jest.spyOn(fullnode.api, 'get'), add an afterEach hook that calls jest.restoreAllMocks() so the spy on fullnode.api.get is cleared between tests and prevents cross-test leakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/wallet-service/tests/fullnode.test.ts`:
- Around line 4-53: Add test teardown to the describe('version') suite to
restore spies after each test: in the same describe block that contains tests
calling jest.spyOn(fullnode.api, 'get'), add an afterEach hook that calls
jest.restoreAllMocks() so the spy on fullnode.api.get is cleared between tests
and prevents cross-test leakage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60830d12-ee76-45b4-a177-edb9e362eac9
📒 Files selected for processing (1)
packages/wallet-service/tests/fullnode.test.ts
Adds `afterEach(() => jest.restoreAllMocks())` to the `describe('version', ...)`
block so the `jest.spyOn(fullnode.api, 'get')` set up by each test is reverted
between tests. Prevents implicit cross-test leakage as the suite grows; aligns
with the cleanup pattern used in tests/auth.readonly.test.ts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| import { FullnodeVersionSchema } from '@src/schemas'; | ||
| import { defaultTestVersionData } from '@tests/utils'; | ||
|
|
||
| // Regression guard for the testnet outage that motivated PR 420: the |
There was a problem hiding this comment.
We should not reference the PR solving the outage if possible, the comment should document the core issue this addresses instead.
I think a link to an issue describing the problem would be good if the core issue was too complex to describe in the comment.
| genesis_tx1_hash?: string, | ||
| genesis_tx2_hash?: string, | ||
| native_token?: { name: string, symbol: string }; | ||
| native_token?: { name: string, symbol: string, version?: number }; |
There was a problem hiding this comment.
I believe we should change the validation schema to allow more fields than the required ones.
So if the fullnode sent the right fields with a couple of more ones like "abc": "asdasd", and "anotherTokenProp": "random"
It shoudn't break the lambda, only if one of the required is missing.
There was a problem hiding this comment.
I think this is a good subject for debate, but if I took this decision only in this native_token field it would leave the rest of the validations following a different, stricter pattern.
This should be done in another PR in my opinion.
Drop the PR 420 reference per review feedback
3 participants
Motivation
Completes the follow-up work after #420, which was a one-line emergency bugfix (allowing
native_token.versionon the fullnode/versionpayload). The fix shipped narrow on purpose; both CodeRabbit and Copilot flagged that it left loose ends — TypeScript typing and tests. This PR closes those out, scoped tightly tonative_token.version.Changes
FullNodeApiVersionResponse.native_tokengainsversion?: numberso the TS type matches the Joi schema. Theas FullNodeApiVersionResponsecast infullnode.tsis dropped (it was masking the drift;Joi.object<T>()already types the validator's output).tests/schemas.test.ts, 4 cases) — first direct coverage ofFullnodeVersionSchema, all scoped to thenative_token.versionfield:native_token.version: 0) validates.native_token.version) validates.native_tokenfail validation (design intent — keep nested object strict).native_token.version(e.g.1.5) fails validation.fullnode.version()tests (extendtests/fullnode.test.ts) — happy-path and schema-rejection cases that verify the schema wiring insidefullnode.tsactually runs and throws. Rejection case uses anative_token.versionviolation.Out of scope (intentionally)
A live-fullnode contract test (that would fail CI when the
/versionresponse drifts) was considered and removed from this PR, since there is no existing precedent for tests making real HTTP calls in the wallet-service workspace — PR #394 went the other direction. That can be a separate PR if we want to introduce that pattern.Acceptance Criteria
FullnodeVersionSchemaaccepts the post-PR 420native_tokenpayload and rejects misuse of the newversionfield; covered by unit tests.FullNodeApiVersionResponsereflects the schema, removing the type cast that was hiding the drift.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets mergedSummary by CodeRabbit
New Features
Refactor
Tests